Initial revision of enterprise MCP allowlist support,#309356
Initial revision of enterprise MCP allowlist support,#309356
Conversation
Co-authored-by: Copilot <copilot@github.com>
There was a problem hiding this comment.
Pull request overview
Adds a first pass of enterprise MCP allowlist enforcement by flowing enterprise registry metadata from the default account policy into MCP startup/connection logic, and by introducing a workbench service that fetches and evaluates an enterprise allowlist.
Changes:
- Extend default account policy data to include enterprise MCP allowlist entries derived from
copilot/mcp_registry. - Introduce
IMcpAllowListService+ implementation to fetch/cache allowlisted server fingerprints and gate MCP server launches. - Gate MCP autostart and connection resolution on allowlist readiness / evaluation, and add
@github/mcp-registryfor fingerprint computation.
Show a summary per file
| File | Description |
|---|---|
| src/vs/workbench/services/accounts/browser/defaultAccount.ts | Extracts enterprise allowlist entries from MCP registry response into policy data. |
| src/vs/workbench/contrib/mcp/common/mcpService.ts | Waits for allowlist readiness before autostarting MCP servers. |
| src/vs/workbench/contrib/mcp/common/mcpRegistry.ts | Gates resolveConnection() on allowlist evaluation; computes server fingerprint via SDK. |
| src/vs/workbench/contrib/mcp/common/mcpAllowListService.ts | New service to fetch/merge/cache enterprise allowlist fingerprints and evaluate server permission. |
| src/vs/workbench/contrib/mcp/browser/mcp.contribution.ts | Registers the allowlist service singleton. |
| src/vs/workbench/contrib/git/common/utils.ts | Exports parseRemoteUrl for allowlist repo-scoping logic. |
| src/vs/platform/mcp/common/mcpAllowListService.ts | New platform-level allowlist service interface and state enum. |
| src/vs/base/common/defaultAccount.ts | Adds IMcpAllowlistEntry and stores allowlist entries on policy data. |
| package.json | Adds @github/mcp-registry dependency. |
| package-lock.json | Locks @github/mcp-registry dependency. |
Copilot's findings
Comments suppressed due to low confidence (2)
src/vs/workbench/contrib/mcp/common/mcpAllowListService.ts:261
AllowListis aSet<string>, but it's being persisted viaJSON.stringify({ value }).JSON.stringify(new Set([...]))serializes to{}, so the cached allowlist will be corrupted on disk and later reads will not return aSet. Persist a JSON-friendly representation (e.g. an array of strings plus a marker for "not applicable"), and reconstruct aSetwhen reading from storage.
private async loadAllowList(query: RegistryQuery, authToken: LazyStatefulPromise<string>, token: CancellationToken): Promise<AllowList> {
const { entry, repo } = query;
const storageKey = `${ALLOWLIST_CACHE_KEY}.${entry.ownerId}${repo ? `.${repo}` : ''}`;
const cached = this.storageService.getObject<{ value: AllowList }>(storageKey, StorageScope.APPLICATION);
if (cached) {
this.logService.debug(`[McpAllowlist] Loaded allowlist for ${repo ?? '<none>'} from cache`);
return cached.value;
}
const value = await this.fetchAllowList(query, authToken, token);
this.storageService.store(storageKey, JSON.stringify({ value }), StorageScope.APPLICATION, StorageTarget.MACHINE);
return value;
src/vs/workbench/contrib/mcp/common/mcpRegistry.ts:595
_checkAllowlistawaitswaitForReady()without handling rejection. Since the allowlist service can cancel/reject its readiness promise on load failure, this can throw and makeresolveConnection()fail with an exception rather than cleanly treating the allowlist asUnavailable/"cannot verify". Handle errors fromwaitForReady()(or ensurewaitForReady()never rejects) so server resolution behavior remains deterministic.
if (this._mcpAllowlistService.state === McpAllowListState.Loading) {
await raceTimeout(this._mcpAllowlistService.waitForReady(), 10_000);
}
- Files reviewed: 9/10 changed files
- Comments generated: 5
| // Enterprise allowlist gate: wait for allowlist data and check if server is permitted | ||
| const allowlistResult = await this._checkAllowlist(definition); | ||
| if (allowlistResult !== true) { | ||
| if (opts.errorOnUserInteraction) { | ||
| throw new UserInteractionRequiredError('allowlist'); | ||
| } | ||
| this._notificationService.notify({ | ||
| severity: Severity.Warning, | ||
| message: allowlistResult.value, | ||
| }); | ||
| return undefined; | ||
| } |
There was a problem hiding this comment.
Enterprise allowlist gating is now part of resolveConnection(), but there are no tests covering allowed/blocked/loading/unavailable outcomes. Since this file already has a dedicated test suite, add tests that stub the allowlist service and verify (1) allowed fingerprints proceed, (2) blocked fingerprints prevent connection and surface a warning / UserInteractionRequiredError('allowlist') when errorOnUserInteraction is set, and (3) unavailable/loading states behave as intended.
This issue also appears on line 592 of the same file.
There was a problem hiding this comment.
How does a user know if an installed MCP server is allowed or not by looking at it in the extensions view?
I think we do the access validation in the mcp workbench service which is the source for all installed mcp servers and disable them if they are not valid. Please check here -
vscode/src/vs/workbench/contrib/mcp/browser/mcpWorkbenchService.ts
Lines 802 to 823 in 503edcb
Co-authored-by: Copilot <copilot@github.com>
Co-authored-by: Copilot <copilot@github.com>
joshspicer
left a comment
There was a problem hiding this comment.
Looks reasonable to me! Would be great to get @sandy081 's 👀 if possible
|
A change has been made to the |
sandy081
left a comment
There was a problem hiding this comment.
Very good work on this, I liked how the overall changes look. I left some comments on the implementation. Thanks.
| // Enterprise allowlist gate: wait for allowlist data and check if server is permitted | ||
| const allowlistResult = await this._checkAllowlist(definition); | ||
| if (allowlistResult !== true) { | ||
| if (opts.errorOnUserInteraction) { | ||
| throw new UserInteractionRequiredError('allowlist'); | ||
| } | ||
| this._notificationService.notify({ | ||
| severity: Severity.Warning, | ||
| message: allowlistResult.value, | ||
| }); | ||
| return undefined; | ||
| } |
There was a problem hiding this comment.
How does a user know if an installed MCP server is allowed or not by looking at it in the extensions view?
I think we do the access validation in the mcp workbench service which is the source for all installed mcp servers and disable them if they are not valid. Please check here -
vscode/src/vs/workbench/contrib/mcp/browser/mcpWorkbenchService.ts
Lines 802 to 823 in 503edcb
Fixes https://github.com/microsoft/vscode-internalbacklog/issues/6959